Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Performance: Fixes query improvement to load values lazily #2869

Merged
merged 13 commits into from
Nov 16, 2021
Merged

Performance: Fixes query improvement to load values lazily #2869

merged 13 commits into from
Nov 16, 2021

Conversation

sourabh1007
Copy link
Contributor

@sourabh1007 sourabh1007 commented Nov 9, 2021 •
edited
Loading

Description

Loading values lazily in CosmosQueryClientCore in order to avoid execution on each request .

Locally, Performance tests shows below number :

Without This PR
master -2

With this PR
changes-3

Method Type Master RPS (op/s) Master Memory (KB) PR RPS (op/s) PR Memory (KB) %Change(op/s) % Change Memory (KB)
QuerySinglePartitionOnePage Stream 45.271 982 45.978 979 0.015617062 0.953179226
QuerySinglePartitionMultiplePage Stream 2.439 5778 7.621 5762 2.124641246 0.998681031
QuerySinglePartitionOnePage Stream 251.309 977 263.758 974 0.049536626 0.730032753
QuerySinglePartitionMultiplePage Stream 42.53 5757 43.474 5741 0.022196097 0.992448497
QuerySinglePartitionOnePage OfT 39.749 2158 40.842 2155 0.027497547 0.981074143
QuerySinglePartitionMultiplePage OfT 6.573 12832 6.732 12821 0.024189868 0.999475374
QuerySinglePartitionOnePage OfT 141.874 2153 149.687 2150 0.055069992 0.930475151
QuerySinglePartitionMultiplePage OfT 20.259 12814 24.913 12797 0.22972506 0.998055798
QuerySinglePartitionOnePage OfTCustom 38.938 2160 40.109 2157 0.03007345 0.981431019
QuerySinglePartitionMultiplePage OfTCustom 6.553 12846 6.725 12829 0.02624752 0.999476491
QuerySinglePartitionOnePage OfTCustom 143.58 2153 149.306 2150 0.039880206 0.930652113
QuerySinglePartitionMultiplePage OfTCustom 20.61 12821 24.847 12797 0.205579816 0.998062008
QuerySinglePartitionOnePage Diagnostics 38.772 220 39.512 2218 0.019085938 0.8204
QuerySinglePartitionMultiplePage Diagnostics 6.497 13181 6.611 13174 0.01754656 0.999498445
QuerySinglePartitionOnePage Diagnostics 139.653 2213 146.873 2211 0.05169957 0.933631722
QuerySinglePartitionMultiplePage Diagnostics 20.65 13093 21.416 13097 0.037094431 0.998364317
QuerySinglePartitionOnePage Telemetry 38.692 2161 39.961 2158 0.032797478 0.981508098
QuerySinglePartitionMultiplePage Telemetry 6.597 12847 6.731 12830 0.020312263 0.999476064
QuerySinglePartitionOnePage Telemetry 148.896 2156 153.916 2153 0.033714808 0.92861039
QuerySinglePartitionMultiplePage Telemetry 23.925 12846 25.82 12808 0.079205852 0.997990036

Here, you can notice the improvement in total memory allocated and Op/s across the board.

Type of change

  • [] Bug fix (non-breaking change which fixes an issue)

Closing issues

Fixes 2,3 of the 3 issues: #2815

@sourabh1007 sourabh1007 force-pushed the users/sourabhjain/queryperffix2 branch 2 times, most recently from dce469c to 334d895 Compare November 9, 2021 23:03
@kirankumarkolli
Copy link
Member

Can you please share the numbers?

Also one clarification: Do lazy materialization expose any gaps in the reference/buffer management?

@sourabh1007
Copy link
Contributor Author

Can you please share the numbers?

Also one clarification: Do lazy materialization expose any gaps in the reference/buffer management?

I have updated the number in the PR description. It showed improvement in memory allocation and op/s for some cases.
Clarification: There should not be any gaps, as we are already using it in SDK at many places. I really don't know how can I check this but if there is any issue in reference/buffer management, it should show up in our performance tests.

FabianMeiswinkel
FabianMeiswinkel requested changes Nov 10, 2021
View reviewed changes
Copy link
Member

@FabianMeiswinkel FabianMeiswinkel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM except for minor comments

Microsoft.Azure.Cosmos/src/Query/Core/Metrics/QueryMetrics.cs Outdated Show resolved Hide resolved
@sourabh1007 sourabh1007 changed the title Performance: Adds query improvement to load values lazily [Internal] Performance: Adds query improvement to load values lazily Nov 11, 2021
j82w
j82w previously approved these changes Nov 11, 2021
View reviewed changes
@sourabh1007 sourabh1007 force-pushed the users/sourabhjain/queryperffix2 branch from 71102c8 to 2bb23af Compare November 11, 2021 15:34
@kirankumarkolli
Copy link
Member

If I am reading right, for multipage interactions with the change is there RPS regression?
Or is it variances if so even for single pages gains are not related to variance?

ealsur
ealsur previously approved these changes Nov 11, 2021
View reviewed changes
Copy link
Member

@ealsur ealsur left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM except some extra comments regarding testing and code duplicity

Microsoft.Azure.Cosmos/src/Query/Core/Pipeline/Pagination/QueryPage.cs Show resolved Hide resolved
Microsoft.Azure.Cosmos/src/Query/Core/Metrics/QueryMetrics.cs Outdated Show resolved Hide resolved
Microsoft.Azure.Cosmos/src/Query/Core/Metrics/QueryMetrics.cs Show resolved Hide resolved
@j82w j82w changed the title [Internal] Performance: Adds query improvement to load values lazily Performance: Fixes query improvement to load values lazily Nov 12, 2021
@sourabh1007 sourabh1007 changed the title Performance: Fixes query improvement to load values lazily Don't review [Testing In Progress] Performance: Fixes query improvement to load values lazily Nov 12, 2021
@sourabh1007
Copy link
Contributor Author

If I am reading right, for multipage interactions with the change is there RPS regression? Or is it variances if so even for single pages gains are not related to variance?

Prev results were from local. Now I ran benchmark on a VM and now you can notice the improvement in RPS and memory allocated also.

@sourabh1007 sourabh1007 force-pushed the users/sourabhjain/queryperffix2 branch from 8582f22 to a8ddeff Compare November 15, 2021 12:05
@sourabh1007 sourabh1007 changed the title Don't review [Testing In Progress] Performance: Fixes query improvement to load values lazily Performance: Fixes query improvement to load values lazily Nov 15, 2021
ealsur
ealsur previously approved these changes Nov 15, 2021
View reviewed changes
Copy link
Member

@ealsur ealsur left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - Just a nit

@sourabh1007 @ealsur
Update Microsoft.Azure.Cosmos/src/Query/Core/Metrics/QueryMetrics.cs
cfe3593 
Co-authored-by: Matias Quaranta <ealsur@users.noreply.github.com>
FabianMeiswinkel
FabianMeiswinkel approved these changes Nov 16, 2021
View reviewed changes
Copy link
Member

@FabianMeiswinkel FabianMeiswinkel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - Thanks

@j82w j82w merged commit b5c24c0 into master Nov 16, 2021
@j82w j82w deleted the users/sourabhjain/queryperffix2 branch November 16, 2021 01:02
@sourabh1007 sourabh1007 linked an issue Dec 24, 2021 that may be closed by this pull request
CPU hotpath originating in CosmosQueryClientCore.GetCosmosElementResponse #2815
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@FabianMeiswinkel FabianMeiswinkel FabianMeiswinkel approved these changes

@j82w j82w j82w approved these changes

@kirankumarkolli kirankumarkolli kirankumarkolli approved these changes

@ealsur ealsur ealsur left review comments

@khdang khdang Awaiting requested review from khdang khdang is a code owner

@kirillg kirillg Awaiting requested review from kirillg kirillg is a code owner

@leminh98 leminh98 Awaiting requested review from leminh98

@neildsh neildsh Awaiting requested review from neildsh neildsh is a code owner

@sboshra sboshra Awaiting requested review from sboshra sboshra is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

CPU hotpath originating in CosmosQueryClientCore.GetCosmosElementResponse
5 participants
@sourabh1007 @kirankumarkolli @ealsur @j82w @FabianMeiswinkel